Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

No call to SetIdle() when terminating #45422

Closed
wants to merge 5 commits into from
Closed

No call to SetIdle() when terminating #45422

wants to merge 5 commits into from

Conversation

ywave620
Copy link
Contributor

@ywave620 ywave620 commented Nov 11, 2022 •
edited
Loading

Fix #45421, where you can find a helpful stack trace

One can argues that v8 is responsable for transfering vm state to EXTERNAL before invoking PerIsolateMessageListener(), if v8 agrees and changes for it, this issue is gone without any change of Node.js. However, SetIdle() is basically for cpu profiling only, it's not necessary when terminating. Removing it is not harmful while otherwise will lead to crash before v8 makes that change.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Nov 11, 2022
@ywave620 ywave620 changed the title No set idle when terminating No call to SetIdle() when terminating Nov 11, 2022
@ywave620
src: no call to SetIdle() when terminating
c0e3840 
Calling SetIdle() when terminating is not harmless.
When node terminates due to an unhandled exception,
v8 preseves the vm state, which is JS and notifies
node through PerIsolateMessageListener(). If node
calls SetIdle() later, v8 complains because it
requires the vm state to either be EXTERNEL or IDLE
when embedder calling SetIdle().
@ywave620
Copy link
Contributor Author

Welcome to join discussion about v8's behavior in https://bugs.chromium.org/p/v8/issues/detail?id=13464

bnoordhuis
bnoordhuis approved these changes Nov 12, 2022
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the caveat that this may end up swapping one bug for another, although it's probably a hypothetical scenario.

I'm thinking of when someone calls isolate->TerminateExecution(), then later on calls isolate->CancelTerminateExecution().

test/parallel/test-unhandled-exception-with-worker-inuse.js Outdated Show resolved Hide resolved
@ywave620 @bnoordhuis
src: no call to SetIdle() when terminating
82f26a2 
Fix typo

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis bnoordhuis added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Nov 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47938/

@ywave620
src: no call to SetIdle() when terminating
6bb4c86 
use fixture function to check no abort
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47960/

@ywave620
Copy link
Contributor Author

Sry, the previous commits forgets to wrap an asserttion around common.nodeProcessAborted. Fixed in the latest one. Please rerun the CI.

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47970/

This was referenced Nov 17, 2022
Open
Open
joyeecheung
joyeecheung approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes here make sense with or without a fix in V8 to update the state to EXTERNAL before invoking the message listener (I think that should also be done - SetIdle() might not be the only victim of it)

santigimeno
santigimeno approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@santigimeno santigimeno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. By the way, this will fix #41107

@github-actions github-actions bot mentioned this pull request Nov 19, 2022
Open
10 tasks
This was referenced Nov 20, 2022
Open
Open
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48055/

This was referenced Nov 22, 2022
Open
Open
@ywave620 ywave620 mentioned this pull request Nov 23, 2022
Merged
@ywave620
Copy link
Contributor Author

I have moved this change to #45596, which includes a fix to another bug about terminating. PTAL.

@ywave620 ywave620 closed this Nov 23, 2022
@github-actions github-actions bot mentioned this pull request Nov 24, 2022
Open
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theanarkh theanarkh theanarkh approved these changes

@legendecas legendecas legendecas approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@santigimeno santigimeno santigimeno approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash if unhandled exception is thrown inside a node program with worker_threads inuse
8 participants
@ywave620 @nodejs-github-bot @bnoordhuis @jasnell @santigimeno @joyeecheung @legendecas @theanarkh